Repository navigation
feat(feature-flags): Evaluate feature flags locally with a polling runtime client - #765
Conversation
[Medium impact] The PR appears safe to merge in the documented stack order. SummaryAdds
|
|
@greptileai review |
1 similar comment
|
@greptileai review |
gjtorikian
left a comment
There was a problem hiding this comment.
The concurrency design here is solid, and the real-server stall and drip tests are great. One change requested before merge, on the deadline timeout dict in _transport.py: it should keep a bounded fallback if anything copies it rather than reading it lazily. Details inline.
| """ | ||
|
|
||
| def __init__(self, deadline: float) -> None: | ||
| super().__init__({phase: None for phase in _PHASES}) |
There was a problem hiding this comment.
_DeadlineTimeouts seeds every phase with None and only overrides get and __getitem__. This works today because httpx2 passes a caller-supplied timeout extension through build_request untouched, and httpcore reads each phase lazily with timeouts.get(...) at the start of that phase (checked against httpx2 2.13.0 and httpcore 1.0.9).
Any consumer that copies the dict, such as a custom transport or a future httpcore change, would see None for every phase. A stalled server would then hang the poller silently: flags go stale, nothing is logged, and close() gives up after its join timeout.
Could you seed each phase with the remaining budget instead of None, and override items, __iter__ and copy to return remaining() as well, so a copying consumer still gets a bounded wait? A short comment naming the httpcore call sites this relies on (connection_pool, connection, http11, http2, http_proxy) would also help whoever does the next httpx2 upgrade.
There was a problem hiding this comment.
Good call, fixed in 1980d27. Each phase now starts with the full budget, and copying the dict any of the usual ways (dict(), {**t}, update(), copy(), copy.copy(), items(), values()) gets the time left instead of None.
A couple small differences from what you suggested:
- iter yields keys so it can't return remaining(), but overriding it is what forces dict()/{**t}/update() through getitem instead of copying the stored values
- copy() returns a new _DeadlineTimeouts with the same deadline, so the copy keeps counting down
Also added the httpcore call sites to the docstring (plus socks_proxy, which reads connect too), and added tests that copy the dict each way on a fake clock. Before the fix they got None for every phase.
|
@greptileai review |
…ntime client FeatureFlagsRuntimeClient polls GET /sdk/feature-flags?payload_version=2 on a daemon thread, keeps the configuration in memory and evaluates flags locally from it. A failed poll keeps the last good configuration and backs off, transient failures are retried within request_timeout, and a 401 stops polling. It supports bootstrap, change, error and status listeners, status and a manual refresh(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A retry inside a refresh waited for whatever Retry-After the server sent, uncapped and possibly negative, while the wait before the next poll was capped at 60 seconds. Both now use the same capped value, as Node does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
close() only joins the background thread, so a manual refresh() that passed its closed check just before close() could still install a new configuration after close() returned, without a change notification. The check and the snapshot update now happen under the lock close() takes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…stop A second close() returned as soon as it saw the client was closed, while the first was still waiting for an in-flight poll, so the second caller could close the WorkOSClient under it. Every caller now waits for the poller thread, as the async client does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The poll's timeout dict stored None for every phase and only answered
get() and [] with the time left. That holds while httpcore reads each
phase lazily, but a custom transport or a later httpcore that copied the
dict would have read None, which means no timeout, so a stalled server
could hang the poller. The stored values now start at the whole budget,
and dict(), {**...}, update(), copy(), copy.copy(), items() and values()
all give the time left.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sync client never awaits what a listener returns, so an async function registered as a listener silently never ran. Raise TypeError at registration instead, including for on_change, on_error and on_status. The check lives in the sync client because the async client accepts coroutine listeners. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1980d27 to
3437e7d
Compare
|
@greptileai review |
Stack. This is #762, split into four PRs for review. Merge them in order; after each merge I'll rebase the next one onto
main.Description
FeatureFlagsRuntimeClientpollsGET /sdk/feature-flags?payload_version=2on a daemon thread and keeps the configuration from #764 in memory. It evaluates flags locally, with no request per check.Retry-Afteris honored up to 60 seconds, by retries inside a refresh and before the next poll.max_retriesand withinrequest_timeout.request_timeoutplus at most one read of a body that stalls. A copy of those timeouts still gets the time left, neverNone, in case a custom transport or a later httpcore copies them instead of reading each phase as it starts.bootstrap,on_change,on_errorandon_statuslisteners,status,get_flag(), and a manualrefresh()that works withoutstart().refresh()itself. Anasync deflistener raisesTypeErrorwhen it's registered, since nothing on the sync client would await it.close()stops polling, and nothing a refresh still in flight does changes the configuration or status after it returns. Every caller waits for the poller, including concurrent ones.on_changeper refresh that installs a new configuration, including the first load.Where to look, in order:
_core.py: state, evaluation and listeners._sync.py:_poll_forever,_refresh_lockedand_drain._transport.py: the time budget._policy.py: backoff and retry decisions.Not in this PR: fork handling comes in #767. Until then, the README says to create the runtime client in each worker after forking. Merge #765 through #767 before the next release PR, so the first release includes fork handling.
How I checked it:
I ran it in our feature flags runtime lab, side by side with the Node SDK, against the same fixture responses. All 80 side-by-side checks pass: 40 for Python, 40 for Node. They cover:
The lab's asyncio checks need feat(feature-flags): Add an async feature flags runtime client #766.
test_transport.pysends the poll to a real local HTTP server. That includes one server that stalls before answering and one that drips the body a byte at a time. In both cases the refresh ends within its budget. Its other tests copy the timeouts every common way on a fake clock and check each phase gets the time left; on the code before that change they gotNone.nox -s cipasses locally: 3,693 tests.Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
The feature flags SDK docs describe the runtime client as Node only. A docs PR adding Python, covering both clients, is still to do.
🤖 Generated with Claude Code